Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support multiple, comma-separated --dns-cluster-ip values #1685

Merged
merged 3 commits into from
Mar 4, 2024

Conversation

ajohnstone
Copy link
Contributor

@ajohnstone ajohnstone commented Feb 23, 2024

Issue #, if available:
#1684

Description of changes:
Support a comma separated list rather than a single scalar value.
bootstrap.sh will override clusterDNS in the kubelet config.

Use case:

This will allow enabling IPVS and node-local by first having node-local and then fall back to the service ip for coredns.

Refer to docs:
https://kubernetes.io/docs/tasks/administer-cluster/nodelocaldns/

If using kube-proxy in IPVS mode, --cluster-dns flag to kubelet needs to be modified to use that NodeLocal DNSCache is listening on. Otherwise, there is no need to modify the value of the --cluster-dns flag, since NodeLocal DNSCache listens on both the kube-dns service IP as well as .

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Testing Done
Modified bootstrap and executed on a re-run.
Tests pass, should these not be part of the build workflow?

+ [[ 0 -eq 0 ]]
+ echo '✅ ✅ All Tests Passed! ✅ ✅'
✅ ✅ All Tests Passed! ✅ ✅
+ exit 0

@ajohnstone ajohnstone changed the title fixes #1684 support multiple clusterDNS for node-local when using ivps fixes #1684 support multiple clusterDNS for node-local when using ipvs Feb 23, 2024
@cartermckinnon
Copy link
Member

Sounds reasonable to me. The linter found an extra space that needs to be removed, @ajohnstone

@ajohnstone
Copy link
Contributor Author

@cartermckinnon thanks removed extra whitespace :)

@ajohnstone
Copy link
Contributor Author

@cartermckinnon mind triggering the tests and approval thanks :)

@cartermckinnon
Copy link
Member

/ci

Copy link
Contributor

github-actions bot commented Mar 4, 2024

@cartermckinnon roger that! I've dispatched a workflow. 👍

Copy link
Contributor

github-actions bot commented Mar 4, 2024

@cartermckinnon the workflow that you requested has completed. 🎉

AMI variantBuildTest
1.23 / al2success ✅success ✅
1.23 / al2023success ✅success ✅
1.24 / al2success ✅success ✅
1.24 / al2023success ✅success ✅
1.25 / al2success ✅success ✅
1.25 / al2023success ✅success ✅
1.26 / al2success ✅success ✅
1.26 / al2023success ✅success ✅
1.27 / al2success ✅success ✅
1.27 / al2023success ✅success ✅
1.28 / al2success ✅success ✅
1.28 / al2023success ✅success ✅
1.29 / al2success ✅success ✅
1.29 / al2023success ✅success ✅

@cartermckinnon cartermckinnon merged commit 029f7c7 into awslabs:main Mar 4, 2024
10 checks passed
@cartermckinnon cartermckinnon changed the title fixes #1684 support multiple clusterDNS for node-local when using ipvs Support multiple, comma-separated --cluster-dns values Mar 4, 2024
@cartermckinnon cartermckinnon changed the title Support multiple, comma-separated --cluster-dns values Support multiple, comma-separated --dns-cluster-ip values Mar 4, 2024
@ajohnstone ajohnstone deleted the patch-1 branch March 4, 2024 23:49
atmosx pushed a commit to gathertown/amazon-eks-ami that referenced this pull request Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants